Cechy dobrego kodu

Nazewnictwo, "suszenie" kodu, złożoność kodu,

prawo Demeter, SOLIDne zasady

dr inż. Aleksander Smywiński-Pohl

apohllo@agh.edu.pl

http://apohllo.pl/dydaktyka/programowanie-obiektowe

$.fn.zakupBiletu = function(options) {
    var zakup_biletu_form = $("#zakup_biletu_form"); 
    var nr_biletu_osoby_towarzyszacej = $("#nr_biletu_osoby_towarzyszacej");          
    var rodzaj_wagonu = $("#rodzaj_wagonu");    
    var dalej_button = $(".dalej_button");    
    var klasa_wagonu = $("#klasa_wagonu");
    var tryb_posel = $("#tryb_posel");
    var przesiadkaPrev = $(".przesiadka_prev");
    var przesiadkaNext = $(".przesiadka_next");     
    var arrow_box_ext_info = $(".arrow_box_ext_info");

    var nextStepPrzesiadka = 0; 
    var ODCINEK = {
        PIERWSZY : 1,
        DRUGI : 2              
    };
    // events 
    dalej_button.unbind('click').click(function (e) {

      var self = $(this);

      var strefaCiszyArr = [52, 11];
      var dlaPodrzDzieckiDoLat6Arr = [3];
      var rowerOferta = 15;
      var SIEDZACE = 1;

      var rodzajWagonu = parseInt($("#rodzaj_wagonu").find("option:selected").val());            
      var rodzajWagonuOdcinek_1 = parseInt($("#rodzaj_wagonu_odcinek_2").val());
      var rower = $("#rodzaj_wagonu").find("option:selected").val();
    }
}

if(BilDodatkoweService().czyNieWybranoCalegoPrzedzialuDlaPsa())

http://kahoot.com

https://bilet.intercity.pl/eic_js/zakup_biletu_plugin.js

Dlaczego ten kod nas śmieszy?

• Mieszanie nazewnictw polskiego i angielskiego: dalej_button, dlaPodrzDzieckiDoLat6Arr, BilDodatkoweService
• Błędy językowe w nazwach zmiennych: dlaPodrz*Dziecki*DoLat6Arr
• Brak jednej konwencji nazewniczej: rodzajWagonuOdcinek_1
• Nazwy metod za bardzo związane z domeną problemu: czyNieWybranoCalegoPrzedzialuDlaPsa
• Kod prezentacji przemieszany z logiką biznesową

  if ((strefaCiszyArr.indexOf(rodzajWagonu) > -1 || strefaCiszyArr.indexOf(rodzajWagonuOdcinek_1) > -1) 
       && !is_przesiadka_zakupowa) {
              openModal = true;
              $("#dalej_modal_strefa_ciszy").show();
            $("#dalej_modal_ok").val(v1);
  }

Dlaczego ten kod nas śmieszy?

• Powtarzający się kod

  var ile_osob = Number($("#liczba_n").find("option:selected").val()) + 
      Number($("#liczba_u").find("option:selected").val()) + 
        Number($("#liczba_u_2").find("option:selected").val());

• Niezrozumiały kod:
  • var strefaCiszyArr = [52, 11];
  • var dlaPodrzDzieckiDoLat6Arr = [3,50];
• "Zabawne" nazwy metod: if (ObslugaKlopotliwychZnizek().validate(this))
• ...
• Ale ten kod ma jedną zaletę: codziennie korzystają z niego tysiące klientów!

Nazewnictwo

There are only two hard things in Computer Science: cache invalidation and naming things.

-- Phil Karlton

public void contents(String s){
    char l = s.toLowerCase().charAt(1);
    if(l == 'd'){
        sectionSearch(s);
    } else if(l == 'r'){
        searchChapter(s);
    } else if(l == 'a'){
        artileSearch(s);
    } else if(l == 'z'){
        betweenSearch(s);
    }
}

Kod wymaga poprawy ponieważ występują w nim zmienne, które nie są pełnymi słowami lub zwrotami. Ponadto metoda contents nie jest czasownikiem i nie oddaje intencji związanej z tym, co kod ma zrobić.

public void executeCommand(String command){
    char commandType = command.toLowerCase().charAt(1);
    if(commandType == 'd'){
        sectionSearch(command);
    } else if(commandType == 'r'){
        searchChapter(command);
    } else if(commandType == 'a'){
        artileSearch(command);
    } else if(commandType == 'z'){
        betweenSearch(command);
    }
}

Kod wymaga poprawy, ponieważ nazwy metod nie są konsekwentne.

public void executeCommand(String command){
    char commandType = command.toLowerCase().charAt(1);
    if(commandType == 'd'){
        searchSection(command);
    } else if(commandType == 'r'){
        searchChapter(command);
    } else if(commandType == 'a'){
        searchArticle(command);
    } else if(commandType == 'z'){
        searchBetweenArticles(command);
    }
}

Poprawiony kod - możliwe jest jeszcze uproszczenie związane z wykorzystaniem konstrukcji switch oraz wyodrębnieniem metody. Ale te modyfikacje nie dotyczą nazewnictwa.

Zalecenia

1. Stosuj tylko nazwy anglojęzyczne
2. Unikaj zmiennych jedno-, dwu-, trzyliterowych, wyjątki
   • zmienne w pętlach
   • zmienne o zakresie jednej linii, najlepiej jednego bloku kodu
3. Używaj nazewnictwa informatycznego, a jeśli kod jest bliski domenie, to używaj poprawnego nazewnictwa z domeny
4. Stosuj nazwy, które można wymówić
5. Nazwy klas powinny zawierać rzeczownik, np. ValueConverter, BillParser, etc.
6. Nazwy metod powinny zawierać czasownik, np. findChapter, drawMenu, sort
7. Jeśli chcesz dodać komentarz wyjaśniający rolę zmiennej, w pierwszej kolejności pomyśl o zmianie jej nazwy.

DRY, czyli wysusz swój kod

• DRY = don't repeat yourself
• nie twórz kodu porzez stosowanie metody kopiuj-wklej

Przykład pierwszy - szereg instrukcji if/else

String command = "r10";

if(command.toLowerCase().charAt(1) == 'd'){
    searchSection(command);
} else if(command.toLowerCase().charAt(1) == 'r'){
    searchChapter(command);
} else if(command.toLowerCase().charAt(1) == 'a'){
    searchArticle(command);
} else if(command.toLowerCase().charAt(1) == 'z'){
    searchBetweenArticles(command);
}

W kodzie wielokrotnie wydobywana jest ta sama informacja command.toLowerCase.charAt(1).

class Commands { 
    public void interpreteCommand(String command){
        if(commandType(command) == 'd') {
            searchSection(command);
        } else if(commandType(command) == 'r') {
            searchChapter(command);
        } else if(commandType(command) == 'a') {
            searchArticle(command);
        } else if(commandType(command) == 'z') { 
            searchBetweenArticles(command);
        }
    }

    private char commandType(String command) {
        return command.toLowerCase().charAt(1);
    }
}

Zamiast wielu if-ów lepiej zastosować konstrukcję switch.

class Commands { 
    public void interpreteCommand(String command){
        switch(commandType(command)) {
            case 'd' :
                searchSection(command);
                break;
            case 'r' :
                searchChapter(command);
                break;
            case 'a' :
                searchArticle(command);
                break;
            case 'z' :
                searchBetweenArticles(command);
                break;
        }
    }
        
    private char commandType(String command) {
        return command.toLowerCase().charAt(1);
    }
}

Ulepszony, przejrzysty kod.

Przykład drugi - powtarzające się bloki kodu

switch(options[0]) {
    case "konstytucja" :
        ParserKonstytucja parserK = new ParserKonstytucja(options);
        parserK.parse();
        if(options.getWhatToDisplay().equals("całość"))
            parserK.write();
        if(options.getWhatToDisplay().equals("element")){
            Element element = parserK.konstytucja;
            // ...
        break;
    case "uokik":
        ParserUokik parserU = new ParserUokik(options);
        parserU.parse();
        if(options.getWhatToDisplay().equals("całość"))
            parserU.write();
        if(options.getWhatToDisplay().equals("element")){
            Element element = parserU.uokik;
            // ...
        break;
}

W kodzie występuje powtórzenie wynikające z faktu użycia dwóch różnych klas parsera, które nie posiadają wspólnego interfejsu lub klasy, z której mogłyby dziedziczyć.

interface BillParser {
    parse();
    displayAll();
    displaySingleElement(ElementId elementId);
    //...
}

class ConstitutionParser implements BillParser {
    //...
}

class RegularBillParser implements BillParser {
    //...
}

Wspólny interfejs.

BillParser parser;

switch(options[0]) {
    case "konstytucja" :
        parser = new ConstitutionParser(options);
        break;
    case "uokik":
        parser = new RegularBillParser(options);
        break;
}
        
parser.parse();
if(options.getWhatToDisplay().equals("całość"))
    parser.displayAll();
if(options.getWhatToDisplay().equals("element")){
    ElementId id = getElementId(options);
    parser.displaySingleElement(id);

Ulepszony kod - dzięki wspólnemu interfejsowi, wystarczy zbudować instancję odpowiedniej klasy, ktróą później można używać dokładnie w ten sam sposób.

Przykład trzeci - powtarzające się struktury kodu

class JudgmentDatabase {
    public String showHelp() {
        return "rubrum - wyświetla metadane sparwy\n" +
            "content - wyświetla treść sprawy\n" +
            "judge - wyświetla liczbę spraw sędziego\n" +
            "months - wyświetla statystyki miesięczne\n"
    }
}

class JudgmentDatabase {
    public String runCommand(String command, String[] args){
        if(invalidArguments(command, args))
            throw new IllegalArgumentException("...");
        switch(command){
            case 'rubrum':
                return rubrumForId(args[0]);
            case 'content':
                return contentForId(args[0]);
            case 'judge':
                return judgeWithNameAndSurname(args[0], args[1]);
            case 'months':
                return monthlyStatistics();
        }
    }
    
    public boolean invalidArguments(String command, String[] args){
        switch(command){
            case 'rubrum':
            case 'content':
                return args.lenght != 1;
            case 'judge':
                return args.lenght != 2;
            case 'months':
                return args.lenght != 0;
        }
        return true;
    }
}

interface Command {
    String getName();
    String getDescription();
    String runCommand(String[] args);
    int requiredArgumentsCount();
}

class RubrumCommand implements Command {
    public String getName() { return "rubrum"; }
    public String getDescription() { return "wyświetla metadane sparwy"; }
    public String runCommand(String[] args) {
        return rubrumForId(args[0]);
    }
    public int requiredArgumentsCount() { return 1; }
    
    private String rubrumForId(String id){
        // ...
        return "";
    }
}

class ContentCommand implements Command {
    public String getName() { return "content"; }
    public String getDescription() { return "wyświetla treść sparwy"; }
    public String runCommand(String[] args) {
        return rubrumForId(args[0]);
    }
    public int requiredArgumentsCount() { return 1; }
    
    private String contentForId(String id){
        // ...
        return "";
    }
}

class JudgmentDatabase {
    private Map<String,Command> commands = new HashMap<String, Command>();
    
    public JudgmentDatabase() {
        for(Command command : List.of(new RubrumCommand(), new ContentCommand())) {
            commands.put(command.getName(), command);
        }
    }

    public String runCommand(String command, String[] args){
        if(getCommand(command).requiredArgumentsCount() == args.length) {
            return getCommand(command).runCommand(args);
        } else {
            throw new IllegalArgumentException("...");
        }
    }
    
    private Command getCommand(String commandName){
        if(commands.containsKey(commandName)){
            return commands.get(commandName);
        } else {
            throw new IllegalArgumentException("Invalid command: " + commandName);
        }
    }
}

Zalecenia

1. Pisz testy - łatwiej będzie można usuwać powtarzający się kod.
2. Po skopiowaniu i modyfikacji fragmentu kodu, sprawdź, czy nie ma elementów wspólnych. Jeśli istnieją - przenieś je do wspólnych metod, klasy nadrzędnej lub klasy, do której możesz oddelegować odpowiedzialność.
3. Szukaj podobieństw między klasami - jeśli istnieją, wyodrębnij interfejs lub klasę nadrzędną.
4. Staraj się wyodrębniać klasy, wszędzie tam, gdzie zbiór metod lub metoda, tworzą zamkniętą całość.

Złożoność kodu

Przykład pierwszy - wielokrotne zagnieżdżenie kodu

case 4:{
    for(Chapter chapter : chapters){
        for(Article article : chapter.getArticles()){
            if(article.getName().equals(arguments.get(0))){
                for(Section section : article.getSections()){
                    if(section.getName().equals(arguments.get(1))){
                        for(SubSection subSection : section.getSubSections()){
                            if(subSection.getName().equals(arguments.get(2))){
                                for(Letter letter : subSection.getLetters()){
                                    if(letter.getName().equals(arguments.get(3))){
                                        letter.writeList(0);
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }
    break;
}

W kodzie wielokrotnie powtarza się ta sama para wywołań - iterowanie po elementach niższego rzędu oraz porównywanie nazwy z selektorem.

class Node {
    private String id;
    private List<Node> children; 
    
    public void print(List<String> path) {
        if(path.length <= 0) {
            return;
        }
        if(this.id.equals(path.get(0))) {
            if(path.length > 1) {
                for(Node child : children) {
                    child.print(path.subList(1, path.length-1));
                }
            } else {
                this.printContent();                
            }
        }
    }
    
    private void printContent() {
        //...
    }
}

Dzięki wprowadzeniu wspólnego typu oraz wykorzystaniu funkcji rekurencyjnej kod uległ znacznemu uproszczeniu.

Przykład drugi - kod obsługi błędu przemieszany z kodem właściwym

if(args[0].equals("konstytucja") || args[0].equals("uokik")){
    if(args[1].equals("treść")) {
        parsedOptions.setContentOrTable(true);
    } else if(args[1].equals("spis")){
        parsedOptions.setContentOrTable(false);
    } else {
        throw new IllegalArgumentException("Bledne dane: " + args[1]);
    }
    if(args[2].equals("całość") || args[2].equals("element") ||  args[2].equals("przedział")){
        parsedOptions.setWhatToDisplay(args[2]);
        // ...
    } else {
        throw new IllegalArgumentException("Bledne dane: " + args[2]);
    }
} else {
    throw new IllegalArgumentException("Bledne dane: " + args[0]);
}

Obsługa błedów przeplata się ze zwykłym kodem, co powoduje zagnieżdżenie wielu if-ów.

if(!(args[0].equals("konstytucja") || args[0].equals("uokik"))){
    throw new IllegalArgumentException("Arg 0 should be one of 'konstytucja' 'uokik', but was: " + args[0]);
}
if(!(args[1].equals("treść") || args[1].equals("spis"))) {
    throw new IllegalArgumentException("Arg 1 should be one of 'treść' 'spis', but was: " + args[1]);
}

if(!(args[2].equals("całość") || args[2].equals("element") ||  args[2].equals("przedział"))){
    throw new IllegalArgumentException("Arg 2 should be one of 'całość' 'element' 'przedział', but was: " + args[1]);
}

parsedOptions.setContentOrTable(args[1].equals("treść"));
parsedOptions.setWhatToDisplay(args[2]);

Dzięki przesunięciu obsługi błędów na początek metody, możliwe jest znaczne uproszczenie kodu, ponieważ można założyć, że wszystkie warunki poprawności argumentów zostały już zweryfikowane. Kod można jeszcze poprawić, korzystając ze zbiorów i stałych.

Set<String> VALID_BILLS = new HashSet(List.of("konstytucja", "uokik"));
Set<String> VALID_COMMANDS = new HashSet(List.of("treść", "spis"));
Set<String> VALID_RANGES = new HashSet(List.of("całość", "element", "przedział"));

String[] args = List.of("konstytucja", "treść", "całość").toArray(new String[0]);

if(!VALID_BILLS.contains(args[0])){
    throw new IllegalArgumentException("Arg 0 should be one of " + VALID_BILLS + ", but was: " + args[0]);
}

if(!VALID_COMMANDS.contains(args[1])){
    throw new IllegalArgumentException("Arg 1 should be one of " + VALID_COMMANDS + ", but was: " + args[1]);
}

if(!VALID_RANGES.contains(args[2])){
    throw new IllegalArgumentException("Arg 2 should be one of " + VALID_RANGES + ", but was: " + args[2]);
}

parsedOptions.setContentOrTable(args[1].equals("treść"));
parsedOptions.setWhatToDisplay(args[2]);

Set<String> VALID_BILLS = new HashSet(List.of("konstytucja", "uokik"));
Set<String> VALID_COMMANDS = new HashSet(List.of("treść", "spis"));
Set<String> VALID_RANGES = new HashSet(List.of("całość", "element", "przedział"));

List<Set<String>> valid_arguments = List.of(VALID_BILLS, VALID_COMMANDS, VALID_RANGES);

String[] args = List.of("konstytucja", "treść", "całość").toArray(new String[0]);

for(int i = 0; i < args.length; i++){
    if(!valid_arguments.get(i).contains(args[i])){
        throw new IllegalArgumentException("Arg " + i + " should be one of " + valid_arguments.get(i) + 
            ", but was: " + args[i]);
    }
}

//parsedOptions.setContentOrTable(args[1].equals("treść"));
//parsedOptions.setWhatToDisplay(args[2]);

---------------------------------------------------------------------------
java.lang.IllegalArgumentException: Arg 2 should be one of [przedział, całość, element], but was: ccałość
	at .(#62:1)

Prawo Demeter (Law of Demeter = LoD)

Inaczej:

• Zasada minimalnej wiedzy
• Reguła ograniczonej interakcji

if(matchPatterns(lines.get(i)).getStructure().getGrading() > 
                prev.getData().getStructure().getGrading()) {
    // ...
}

Klasa, w której występuje to porównanie musi znać strukturę klasy Tree.Node, Act oraz Structure. To zdecydowanie zbyt wiele. W myśl zasady LoD, powinna wykorzystać wiedzę wyłącznie do obiektów, które zostały do niej przekazana, bądź są składowymi klasy.

class Tree {
    class Node<T> {
        private T data;
        
        public T getData() {
            return data;
        }
    }
}

class Act {
    private Structure structure;
    
    public Structure getStructure() {
        return structure;
    }
}

class Structure {
    private int grading;
    
    public int getGrading() {
        return grading;
    }
}

Istniejące klasy w klasyczny sposób udostępniają swoje dane zewnętrzne, poprzez metody dostępowe. W istocie rzeczy, klasy takie nie są obiektami, ale strukturami danych, ponieważ nie przejawiają złożonego zachowania.

class Act {
    public int compare(Act other) {
        this.structure.compare(other.structure);
    }
}

class Structure {
    public int compare(Structure other) {
        if(this.grading > other.grading) {
            return 1;
        } else if(this.grading < other.grading) {
            return -1
        } else {
            return 0;
        }
    }
}

if(matchPatterns(lines.get(i)).compare(prev.getData()) > 0) {
    // ...
}

Rozwiązanie polega na delegowaniu wywołania compare do własnych składowych. Dzięki temu - jeśli np. w klasie Act nastąpiłaby jakaś zmiana związana z polem structure, klasa, która wywołała metodę compare nie musiałaby ulec zmianie.

SOLIDne zasady

• Single responsibility principle
• Open/close principle
• Liskov substitution principle
• Interface segregation principle
• Dependency inversion principle

Single responsibility principle - zasada pojedynczej odpowiedzialności

a class should have only a single responsibility (i.e. changes to only one part of the software's specification should be able to affect the specification of the class).

• klasa powinna ulegać zmianie tylko z powodu zmiany pojedynczego wymagania
• jeśli zmiana dwóch wymagań może doprowadzić do zmiany jednej klasy to łamie ona zasadę SRP

class ActNode {
    private NodeType type;
    private String title;
    private List<String> content;
    private List<ActNode> children;
    
    public ActNode getChild(String id) {
        // ...
    }
    
    public List<ActNode> getChildren() {
        ///
    }
    
    public String contentWithChildren() {
        // ...
    }
    
    public String tableOfContents() {
        // ...
    }
    
    // ... after some time
    
    public String htmlTableOfContents() {
        // ...
    }
    
    public String jsonTableOfContents() {
        // ...
    }
}

interface ActPresenter {
    public String content();
    
    public String contentWithChildren(int level);
}

class TextActPresenter implements ActPresenter {
    public TextActPresenter(ActNode node){
        // ...
    }
    
    public String content() {
        // ...
    }
    
    public String contentWithChildren(int level) {
        // ...
    }
}

class TocActPresenter implements ActPresenter {
    public TocActPresenter(ActNode node) {
        // ...
    }
    
    public String content() {
        // ...
    }
    
    public String contentWithChildren(int level) {
        // ...
    }
}

class HtmlActPresenter implements ActPresenter {
    public HtmlActPresenter(ActNode node) {
        // ...
    }
    
    public String content() {
        // ...
    }
    
    public String contentWithChildren(int level) {
        // ...
    }
}

Open/closed principle

"software entities … should be open for extension, but closed for modification."

• typ powinien być otwarty na rozszerzanie, czyli dodanie nowej implementacji tego typu
• typ powinien być zamknięty z względu na modyfikację, czyli modyfikację istniejących cechy użytkowych tego typu

class LegalActParser {
    private Options options;
    
    public LegaActParser(Options options) {
        this.options = options;
    }
    
    public ActNode parse(List<String> text) {
        if(this.options.type == ActType.Constitution) { 
            parseContitution();
        } else if(this.options.type == ActType.Uokik) {
            parseUokik();
        }
    }
    // ...
}

Ten typ nie jest zamknięty ze względu na modyfikację, ponieważ dodanie nowego rodzaju ustawy będzie wymagało zmiany istniejącej implementacji.

abstract class LegalActParser {
    private Options options;
    
    public LegalActParser(Options options) {
        this.options = options;
    }
    
    public abstract ActNode parse();
}

class ConstitutionActParser extends LegalActParser {
    public ActNode parse() {
        // ...
    }
}

class UokikActParser extends LegalActParser {
    public ActNode parse() {
        // ...
    }
}

interface Collection<E> {
    // "złamanie" zasady Open/Close
    public default Stream<E> stream() {
        // ...
    }
}

Liskov substitution principle

"objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program."

• powinna istnieć możliwość zamiany obiektów jednej klasy, obiektami klasy z niej dziedziczącej bez utraty poprawności działania programu
• klasa nadrzędna powinna być zastępowalna klasami, które z niej dziedziczą

interface List<E> {
    /**
    * Inserts the specified element at the specified position in this list (optional operation).
    */
    public boolean add(int index, E element);
}

class Collections {
    static <T> List<T> unmodifiableList(List<? extends T> list) {
        // ...
    }
}

class ActNode {
    private List<ActNode> children;
    
    public ActNode(List<ActNode> children) {
        this.children = children;
    }
    
    public add(Child node) {
        this.children.add(node);
    }
}

class ActNode {
    // ???
    public add(Child node) {
        try {
            this.children.add(node);
        } catch(UnsupportedOperationException ex) {
            System.out.println("Cannot add children to the node");
        }
    }
}

Interface segregation principle

"many client-specific interfaces are better than one general-purpose interface."

• interfejsy powinny być zorganizowane w taki sposób, aby zbiory cech potrzebnych w różnych, niezależnych kontekstach nie były zgromadzone w jednym interfejsie

public interface IWorldMap {
    boolean canMoveTo(Position position);
    boolean place(Car car);
    void run(MoveDirection[] directions);
    boolean isOccupied(Position position);
    Object objectAt(Position position);
}

public interface IWorldMap {
    boolean canMoveTo(Position position);
    boolean place(Car car);
    boolean isOccupied(Position position);
    Object objectAt(Position position);
}

public interface IGameRunner {
    void run(MoveDirection[] directions);
}

Dependency inversion principle

one should "depend upon abstractions, (not) concretions."

• jeśli interfejs ma wiele implementacji, to klasa korzystająca z nich powinna deklarować ten interfejs, a nie odwoływać się do konkretnych implementacji

switch(options[0]) {
    case "konstytucja" :
        ParserKonstytucja parserK = new ParserKonstytucja(options);
        // ...
        break;
    case "uokik":
        ParserUokik parserU = new ParserUokik(options);
        // ...
        break;
}

BillParser parser;

switch(options[0]) {
    case "konstytucja" :
        parser = new ConstitutionParser(options);
        break;
    case "uokik":
        parser = new RegularBillParser(options);
        break;
}